-
Notifications
You must be signed in to change notification settings - Fork 181
BUILD: Add configurable CUDA architecture targeting with meson built-in options #719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
BUILD: Add configurable CUDA architecture targeting with meson built-in options #719
Conversation
|
👋 Hi michal-shalev! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
meson.build
Outdated
| nvcc_flags += ['-gencode', 'arch=compute_80,code=sm_80'] | ||
| nvcc_flags += ['-gencode', 'arch=compute_90,code=sm_90'] | ||
| nvcc_flags_link = [] | ||
| if get_option('nvcc_gencode') != '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why need this check? wouldn't the for loop below be empty any way if options not set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because with a string option, ''.split(' ') returns [''], so the loop runs once and adds an empty flag. I changed nvcc_gencode to an array and removed this check and split to avoid empty args.
meson_options.txt
Outdated
| option('cudapath_inc', type: 'string', value: '', description: 'Include path for CUDA') | ||
| option('cudapath_lib', type: 'string', value: '', description: 'Library path for CUDA') | ||
| option('cudapath_stub', type: 'string', value: '', description: 'Extra Stub path for CUDA') | ||
| option('nvcc_gencode', type: 'string', value: '-gencode=arch=compute_80,code=sm_80 -gencode=arch=compute_90,code=sm_90', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like can provide meson cuda build flags like this:
meson setup -Dcuda_args="-gencode=arch=compute_90,code=sm_90" \
-Dcuda_link_args="-gencode=arch=compute_90,code=sm_90"
Can you pls check it? so maybe no need to add specific option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested -Dcuda_args and -Dcuda_link_args and it's working.
I've updated the PR to use meson's built-in cuda_args and cuda_link_args instead of adding a custom option.
7e58efd to
01da329
Compare
0bb541f to
07321e7
Compare
Signed-off-by: Michal Shalev <[email protected]>
| '-gencode=arch=compute_80,code=sm_80', | ||
| '-gencode=arch=compute_90,code=sm_90' | ||
| ] | ||
| default_cuda_link_args = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove and use default_cuda_args instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notice that previously we had nvcc_flags and nvcc_flags_link, I did not won't to change that logic, only to use the built-in meson option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i mean that default_cuda_link_args and default_cuda_args are the same, so we can remove the first one and use default_cuda_args line 114 too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvcc_flags and nvcc_flags_link were the same, I can set default_cuda_link_args to default_cuda_args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am suggesting to remove default_cuda_link_args = {} at line 100, and replace all other occurences of default_cuda_link_args by default_cuda_args.
| cuda_args = default_cuda_args | ||
| add_project_arguments(cuda_args, language: 'cuda') | ||
| else | ||
| cuda_args = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess we need to add_project_argument with cuda_args_option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, meson has built-in cuda_args and cuda_link_args options, but if it's not clear I can add another comment here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, but also why cuda_args=[]? i would have assumed we need to pass it to the cuda.compiles() below? or is it implictly passed already? in that case we could remove the args: cuda_args?
|
/build |
| - `compute_80,code=sm_80`: NVIDIA Ampere (A100, RTX 30xx) | ||
| - `compute_86,code=sm_86`: NVIDIA Ampere (RTX 30xx consumer) | ||
| - `compute_89,code=sm_89`: NVIDIA Ada Lovelace (RTX 40xx) | ||
| - `compute_90,code=sm_90`: NVIDIA Hopper (H100, H800, H200) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question 1: do we support Blackwell? Why is it not listed?
Question 2: what should be the value when packaging the wheel, since that needs to cover all platforms where the user may do pip install nixl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I didn't test it on a cluster with Blackwell, and I don't think we'll have time to test it for this release.
- IMO it should stay the same, there are still defaults
3dda4df to
807c8e8
Compare
What?
Add support for configurable CUDA architecture targeting using Meson's built-in
cuda_argsandcuda_link_argsoptions.Why?
Replaces hardcoded CUDA architecture flags with configurable defaults using meson's built-in CUDA argument handling for better flexibility.
How?
cuda_args/cuda_link_argscompiler optionsUsage
See
README.mdfor complete documentation and additional examples.